-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add organization role sync and granting #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pkg/connector/org_role.go
Outdated
| bag, _, err := parsePageToken(pToken.Token, &v2.ResourceId{ResourceType: resourceTypeOrgRole.Id}) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove this and always return an empty next token if ListRoles is not paginated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
pkg/connector/org_role.go
Outdated
| var ret []*v2.Grant | ||
|
|
||
| // First, get teams with this role | ||
| teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, roleID, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will want to use a pagination bag so we can get team assignments/user assignments separately so we paginate properly. An example of how we do this: https://github.com/ConductorOne/baton-okta/blob/81354f306dc69ed74754499a8ea8bb5b234b6943/pkg/connector/group.go#L124
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a new commit to address the pagination
…moved unnecessary page token parsing and streamlined return values for list and grants functions. Improved handling of permission errors and added pagination logic for teams and users.
…d mock GitHub server to handle paginated responses for teams and users, and updated tests to verify correct handling of grants and pagination scenarios.
pkg/connector/org_role.go
Outdated
| if len(teams) == pToken.Size { | ||
| pageToken, err := bag.NextToken(fmt.Sprintf("%d", page+1)) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
| return ret, pageToken, nil, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we actually set size all the time, and if the token we return is just the page I think it will do the ListTeamsAssignedToOrgRole if theres a second page from ListUsersAssignedToOrgRole. We usually paginate by pushing a page to the bag when we have multiple actions we want to do within a method like here so we can distinguish which action to do
See this example:
https://github.com/ConductorOne/baton-okta/blob/81354f306dc69ed74754499a8ea8bb5b234b6943/pkg/connector/group.go#L124
…dling. Streamlined resource type checks and enhanced grant creation for users and teams, ensuring proper handling of permission errors and returning accurate annotations.
…erification. Replaced role existence check with a direct API request, improved entitlement ID validation, and streamlined request creation for assigning roles to users.
…nRole. Refactor test cases to utilize the new role data
…e organization role handling. Showing the team > org_role entitlement looks good in the UI but provisioning fails since it's expecting team roles not org roles. We need to think about the right approach before emitting these entitlements
| // First verify that the role exists | ||
| req, err := o.client.NewRequest("GET", fmt.Sprintf("orgs/%s/organization-roles/%d", orgName, roleID), nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think we need to do this since the PUT should 404 if it doesn't exist but nbd
Uh oh!
There was an error while loading. Please reload this page.